Frontend login form #110

A-Wodnicki · 2023-08-01T15:15:19Z

A-Wodnicki · 2023-08-01T15:27:53Z

wgrałem nowe zmienne z Figmy ale niektóre są bez jednostek i musiałem kombinować z mnożeniem przez jeden piksel
nie mamy jeszcze łososiowego loga więc wziąłem najbardziej podobne

PixelSculptor · 2023-08-05T09:17:50Z

ui/src/components/LoginForm/LoginForm.module.scss

+  gap: $form-gap;
+  text-align: center;
+  background-color: $alternative-a900;
+  border: 6px solid $primary-p700;


I suggest change border width to rem cause everywhere you're using rem so it's good idea to keep consistency

PixelSculptor · 2023-08-05T09:18:37Z

ui/src/components/LoginForm/LoginForm.module.scss

+  border-radius: 0.625rem;
+  padding: 2.875rem 5.4375rem;
+
+  @media screen and (height > 45rem) {


I am not sure if resolution for media query should be define as rem unit. @marcol13 what do you think?

@Kapson007 yes, you are right. It should be defined in px unit.

PixelSculptor · 2023-08-05T09:20:13Z

ui/src/components/LoginForm/LoginForm.module.scss

+  }
+}
+
+h3 {


I don't know if we are using BEM but imo its better to use class selectors and avoid tag selectors as much as possible to control specificity properly.

PixelSculptor · 2023-08-05T09:22:45Z

ui/src/components/LoginForm/LoginForm.module.scss

+}
+
+.passwordReset {
+  margin-top: -$form-gap;


in the future its better to create another var rather than using like that beacuse now it's not semantic - using var for gaps in place for margins.

marcol13 · 2023-08-07T18:53:49Z

ui/src/components/LoginForm/LoginForm.jsx

+    password: string().required('Pole "Hasło" jest wymagane.')
+  })
+
+  const handleChange = (event) => {


Ciekawe podejście. Powinno działać, ale lepiej użyć do tego useRef. Ten hook nie powoduje rerenderowania się elementu przy każdej zmianie (w tym przypadku na każdy onChange któregokolwiek z inputów), a walidacja w czasie rzeczywistym nie jest nam potrzebna (wystarczy jedynie sprawdzenie poprawności po kliknięciu przycisku). Jest to więc bardziej wydajny sposób na poradzenie sobie z formularzem 😉

https://levelup.gitconnected.com/react-forms-usestate-vs-useref-5cb584cc19fd

Według mnie lepszym podejściem byłoby zrobić refy na każdy input i po kliknięciu przycisku (czyli na onSubmit) odczytywać wartości inputów poprzez .current.value.

https://dev.to/sobhandash/react-forms-and-useref-hook-4p1l

Errory mogą być wtedy nadal przechowywane jako useState (bo będą wymuszały rerenderowanie pewnego fragmentu komponentu — komunikatu o błędzie).

marcol13 · 2023-08-07T18:53:52Z

ui/src/components/LoginForm/LoginForm.jsx

+  }
+
+  return (
+    <div className={styles.view}>


Z tego diva w późniejszym etapie będzie trzeba utworzyć jakiś nowy komponent 😉 Tak, żeby w pliku LoginForm.jsx był sam element form. Na razie niech tak to zostanie, ja się postaram ty zająć 😉

marcol13 · 2023-08-07T18:58:50Z

ui/src/components/LoginForm/LoginForm.jsx

+        </div>
+      </header>
+      <form className={styles.form} ref={loginForm} onSubmit={handleSubmit}>
+        <header>


Wydaje mi się, że w tym miejscu element header jest trochę niepotrzebny. Można go zastąpić zwykłym divem.

marcol13 · 2023-08-07T19:01:16Z

ui/src/components/LoginForm/LoginForm.jsx

+        )}
+        <Button
+          style={styles.button}
+          onClick={() => loginForm.current.onSubmit}


Tutaj, tak jak mówiliśmy na spotkaniu, możesz zmienić definicję komponentu Button, tak aby ten onClick nie był required. Wtedy nie będzie trzeba tu robić niepotrzebnych fikołków 😉

marcol13 · 2023-08-07T19:03:42Z

ui/src/components/LoginForm/LoginForm.jsx

+          <span className={styles.errorSpan}>Nie udało się zalogować</span>
+        )}
+        <Button
+          style={styles.button}


Żeby nie było potem wątpliwości i definicji w SCSS button.button, możesz zmienić nazwę klasy na inną, np. styles.submitButton

Możesz nawet nie zwracać specjalnie uwagi na złe stylowanie tego komponentu, bo Button.jsx jest do poprawy, także nie musisz tego na siłę dostosowywać w tym LoginFormie 😉 Ale oczywiście na razie możesz tak to zostawić 😄

marcol13 · 2023-08-07T19:07:40Z

Btw. świetna robota! Widok formularza wygląda praktycznie identycznie jak na makiecie. Kilka sugestii ode mnie i wcześniej od @Kapson007 😉

PixelSculptor

looks nice, there are some comments to consider with team ;)

PixelSculptor · 2023-08-23T20:09:18Z

ui/src/components/LoginForm/LoginForm.jsx

+        </div>
+      </header>
+      <form className={styles.form} ref={loginForm} onSubmit={handleSubmit}>
+        <h3 className={styles.header__level3}>Zaloguj się do swojego konta</h3>


I think now is better but maybe we should avoid addicting to structure and only write just header or BEM element login__header?

login__header sounds better 😄 header__level3 is a little bit confusing

PixelSculptor · 2023-08-23T20:10:28Z

ui/src/components/LoginForm/LoginForm.module.scss

+  grid-template-rows: auto 1fr;
+  align-items: center;
+  background-color: #282828;
+  color: $base-white;


please mark as TODO to refactor in the future this calculation with mixin

PixelSculptor · 2023-08-23T20:11:03Z

ui/src/components/LoginForm/LoginForm.module.scss

+  background-color: #282828;
+  color: $base-white;
+  font-size: calc($font-size-p * 1px);
+  font-weight: 600;


is there any var for font weight? @marcol13 ?

No, there are no variables for that. I am not sure if it is necessary, because in default font-weight values have specific meaning, e.g. 400 - normal, 600 - semibold.

What do you think about it? Should I add proper tokens for weight? @A-Wodnicki @PixelSculptor

PixelSculptor · 2023-08-23T20:12:57Z

ui/src/components/LoginForm/LoginForm.module.scss

+  background-color: $alternative-a900;
+  border: 0.375rem solid $primary-p700;
+  border-radius: 0.625rem;
+  padding: 2.875rem 5.4375rem;


is it expected to use these specific float values? @marcol13

Some of these values were copied directly from our Figma if they were available to check, I set it to display the values in rem units

okay approve ;) we need to figure out on our meeting how to round up/down these values

We can discuss that on our next meeting 😉 Because maybe not everything should be in rem

PixelSculptor · 2023-08-23T20:14:13Z

ui/src/components/LoginForm/LoginForm.module.scss

+  }
+}
+
+button.button {


you want raise specificity? I think here you don't need button selector

Higher specificity was required to override some rules over button component without using !important

have you tried .form__button?

.form__button is more readable than button.button 😉

PixelSculptor · 2023-08-23T20:14:33Z

ui/src/components/LoginForm/LoginForm.module.scss

+  color: $alternative-a900;
+  font-weight: 600;
+  border-radius: $border-radius-md;
+  padding: 0.5625rem 1.875rem;


same specific value

marcol13

Wygląda to bardzo dobrze! 🎉 Kilka małych komentarzy. Myślę, że jak je poprawisz to będzie można tego Pull Requesta mergować 😉

marcol13 · 2023-08-24T20:27:09Z

ui/src/components/LoginForm/LoginForm.jsx

+    password: string().required('Pole "Hasło" jest wymagane.')
+  })
+
+  const handleChange = (event) => {


Myślę, że teraz, gdy korzystamy z useRef, to ta funkcja nie jest nam za bardzo potrzebna 😉 Walidacja razem z ustawieniem błędów powinna się odbywać tylko po naciśnięciu przycisku. Po zmianie wartości w inputach nie potrzebujemy na bieżąco kontroli nad wartościami wpisywanymi przez użytkownika. Wprowadza to tylko niepotrzebne operacje 😄

Czyli błędy, które wcześniej się pojawiły mają się chować przy submicie? Ta funkcja je chowa jak wpisze się coś do inputa, w którym błąd wystąpił

Aaa rozumiem... Nie przewidywaliśmy w sumie takiego zachowania inputa — myślę, że dla prostoty i optymalizacji nie trzeba tutaj usuwać komunikatu o błędzie po wpisaniu tam czegokolwiek.

marcol13 · 2023-08-24T20:30:42Z

ui/src/components/LoginForm/LoginForm.stories.jsx

@@ -0,0 +1,15 @@
+import React from 'react'


Ogólnie to niekoniecznie potrzebne są nam w Storybooku całe widoki jako historyjki 😉 Bardziej nam będzie zależało, żeby tam znalazły się komponenty, ale kudosy za to, że to postawiłeś 😁 Myślę, że możesz tak to zostawić, ale w przyszłości nie jest wymagane, żebyś to aplikował dla całych stron 😉

A-Wodnicki and others added 4 commits May 16, 2023 21:19

init LoginForm

bf6617d

LoginForm validation errors handling

881d2a9

Merge branch 'frontend' into frontend-login-form

c4afbb2

Login view styling

9841b0d

A-Wodnicki marked this pull request as ready for review August 2, 2023 19:18

A-Wodnicki requested a review from marcol13 August 2, 2023 19:19

A-Wodnicki self-assigned this Aug 3, 2023

A-Wodnicki linked an issue Aug 3, 2023 that may be closed by this pull request

[Frontend] Formularz logowania #81

Open

A-Wodnicki added the frontend Frontend tasks label Aug 3, 2023

PixelSculptor reviewed Aug 5, 2023

View reviewed changes

marcol13 reviewed Aug 7, 2023

View reviewed changes

A-Wodnicki added 4 commits August 22, 2023 13:36

Add logo with accent color

d383515

Make onClick prop in Button component optional

c109728

LoginForm: change input values storage from state to ref

8e6bbc3

Fix LoginForm styling inconsistencies

8aa5a85

A-Wodnicki requested review from marcol13 and PixelSculptor August 22, 2023 12:30

PixelSculptor reviewed Aug 23, 2023

View reviewed changes

marcol13 reviewed Aug 24, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frontend login form #110

Frontend login form #110

A-Wodnicki commented Aug 1, 2023

A-Wodnicki commented Aug 1, 2023

PixelSculptor Aug 5, 2023

PixelSculptor Aug 5, 2023

marcol13 Aug 7, 2023

PixelSculptor Aug 5, 2023

PixelSculptor Aug 5, 2023

marcol13 Aug 7, 2023

marcol13 Aug 7, 2023

marcol13 Aug 7, 2023

marcol13 Aug 7, 2023

marcol13 Aug 7, 2023

marcol13 Aug 7, 2023

marcol13 commented Aug 7, 2023

PixelSculptor left a comment

PixelSculptor Aug 23, 2023

marcol13 Aug 24, 2023

PixelSculptor Aug 23, 2023

PixelSculptor Aug 23, 2023

marcol13 Aug 24, 2023

PixelSculptor Aug 23, 2023

A-Wodnicki Aug 23, 2023

PixelSculptor Aug 24, 2023

marcol13 Aug 24, 2023

PixelSculptor Aug 23, 2023

A-Wodnicki Aug 23, 2023

PixelSculptor Aug 24, 2023

marcol13 Aug 24, 2023

PixelSculptor Aug 23, 2023

marcol13 left a comment

marcol13 Aug 24, 2023

A-Wodnicki Aug 26, 2023

marcol13 Dec 3, 2023

marcol13 Aug 24, 2023

+                }
+              }
+              h3 {

Frontend login form #110

Are you sure you want to change the base?

Frontend login form #110

Conversation

A-Wodnicki commented Aug 1, 2023

A-Wodnicki commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcol13 commented Aug 7, 2023

PixelSculptor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcol13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment